Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TACKLE-671: Application form - Packaging Popover #300

Merged
merged 10 commits into from
Jul 13, 2022
Merged

TACKLE-671: Application form - Packaging Popover #300

merged 10 commits into from
Jul 13, 2022

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Jun 17, 2022

@gildub gildub requested a review from ibolton336 June 17, 2022 11:49
@gildub gildub requested a review from ibolton336 June 27, 2022 10:18
Copy link
Member

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a question mark icon next to the field label and add the tooltip or popover to that? This UX seems odd in that the tooltip is floating to the left of the input box outside of the form and that there is no indication that there is a tooltip present until hovering over the field.
image

@gildub
Copy link
Contributor Author

gildub commented Jun 28, 2022

@ibolton336, the related issue (TACKLE-671) clearly lacks requirements.
To avoid going back and forth we need to make sure requirements are validated by UXD team.
The tooltip position is now centered above by default.
I don't think a question mark makes sense here but again let's get UXD to determine that. The latter can be added in a separate PR.

@ibolton336
Copy link
Member

@ibolton336, the related issue (TACKLE-671) clearly lacks requirements. To avoid going back and forth we need to make sure requirements are validated by UXD team. The tooltip position is now centered above by default. I don't think a question mark makes sense here but again let's get UXD to determine that. The latter can be added in a separate PR.

No problem. Lets wait for the UXD input before moving forward just so we have something to go off of for future tooltips.

@gildub
Copy link
Contributor Author

gildub commented Jun 28, 2022

@ibolton336, thank you for the follow-up with UXD, I have replaced tooltip with popover.

@gildub gildub changed the title TACKLE-671: Application form - Packaging Tooltip TACKLE-671: Application form - Packaging Popover Jun 28, 2022
Copy link
Member

@ibolton336 ibolton336 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something isn't looking quite right here with the spacing and icon size.
image

Also, I think I remember the popover on MTC having a black background? I could be wrong there but seems like the popover is hard to read against the form background. I wonder why the tooltip has a black background and the popover does not.

@gildub gildub requested a review from ibolton336 July 1, 2022 08:37
@gildub
Copy link
Contributor Author

gildub commented Jul 11, 2022

@ibolton336, please re-review with dark popover in place.

@ibolton336
Copy link
Member

ibolton336 commented Jul 11, 2022

@ibolton336, please re-review with dark popover in place.

image

I think the form spacing may be the culprit here, but the label seems cramped underneath the above input box. Also, the icon size still doesn't seem to be scaled correctly. Would love to nail this so we can reuse across the app. Maybe even create a label with popover icon local component if that makes sense?

@gildub
Copy link
Contributor Author

gildub commented Jul 12, 2022

@ibolton336, I updated it wrapping Icon with span and css properties and also replaced Icon with QuestionCircleIcon

@gildub
Copy link
Contributor Author

gildub commented Jul 13, 2022

@ibolton336, I don't know why we have to go all the way to define CSS at top level as we didn't need such approach with tackle. Anyway it looks much better. Thanks

image

@ibolton336
Copy link
Member

@ibolton336, I don't know why we have to go all the way to define CSS at top level as we didn't need such approach with tackle. Anyway it looks much better. Thanks

image

Global CSS is useful if we have modifier classes that are available across widely used components. That way we don't have to import the css again in each file. Is there another approach you had in mind?

@gildub
Copy link
Contributor Author

gildub commented Jul 13, 2022

@ibolton336, I understand the global vs local approach, but that's the thing, in forklift, I don't think we're changing anything locally but as you mentioned we might be using a different PF version.

@gildub gildub merged commit 192c85c into konveyor:main Jul 13, 2022
@gildub gildub deleted the jira-671 branch July 13, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants